Skip to content

Correct Class Segment in spm.preprocess #580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 15, 2013
Merged

Correct Class Segment in spm.preprocess #580

merged 3 commits into from
Jun 15, 2013

Conversation

mick-d
Copy link
Contributor

@mick-d mick-d commented Jun 11, 2013

The spm Segment module (spm_preproc) does not include a modulated version of the structural image input in its outputs, however the NiPype Segment warper expected such an output.

This led to a systematic error being thrown at execution about the modulated file being missing. I corrected this error by removing references to the modulated structural image in the associated Segment code.

@satra
Copy link
Member

satra commented Jun 12, 2013

is this true even if you set save_bias_corrected = True?

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

I'll try that now

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

You are right, no error when save_bias_corrected = True. However why call the bias-corrected image the "modulated version of input image" in the code? I didn't think the so called "modulated" output was actually the bias-corrected image, that's why i proposed this commit.

Then how to deal with this optional output?

@satra
Copy link
Member

satra commented Jun 12, 2013

it's called modulated only from spm's terminology - we could call it bias_corrected_image. in terms of dealing with it:

regarding the outputs, i would recommend removing exists=True from all the outputspec metadata and then determine which files are generated based on the inputs *_output_type and the save_bias_corrected flag.

if isdefined(self.inputs.save_bias_corrected) and self.inputs.save_bias_corrected:
    outputs['bias_corrected_image'] = fname_presuffix(f, prefix='m')

and so on.

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

Great! Should i do this modification and add it here? And change "modulated version of input image" to "bias-corrected image"?

@satra
Copy link
Member

satra commented Jun 12, 2013

if you have the time, that would be great!

@satra
Copy link
Member

satra commented Jun 12, 2013

regarding the bias corrected image, let's add another field and deprecate the old field in a couple of releases. for now we will have two outputs with the same content.

check the example under deprecated and new_name at the end of:

http://nipy.org/nipype/devel/interface_specs.html#common

…bias_corrected boolean is True.

This prevents to throw an error when save_bias_corrected is set to False but Segment still looks for a created bias-corrected output.
@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

The output list was already filtered according to options for the WM, GM and CSF probability maps so i only added the condition you suggested in the same part of code (cf 583 to 587), i hope that's all right. I tested it and it works fine whatever boolean the save_bias_corrected variable variable is set to.

I only saw your second message now so i'll have to add another commit for the old and new field.

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

In which release should it be deprecated? Did you mean in a couple of (sub)releases, say 0.9.3?

@satra
Copy link
Member

satra commented Jun 12, 2013

i would say 0.10

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

Thanks, i did this change in SegmentOutputSpec:
modulated_input_image = File(exists=True, deprecated='1.0', new_name='bias_corrected_image', desc='bias-corrected version of input image')
bias_corrected_image = File(exists=True, desc='bias-corrected version of input image')

I'm not familiar with this deprecation ability, so what to do for the new line of code in Segment? Should i just keep it with the old variable like below:
if isdefined(self.inputs.save_bias_corrected) and self.inputs.save_bias_corrected:
outputs['modulated_input_image'] = fname_presuffix(f, prefix='m')

Note that as indicated in my post above i placed this line in the Segment class as code was already implemented there to modify the effective output.

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

And many thanks for your help. I know you spend as much time or even more explaining me things in this discussion that you would modifying directly the code. But hopefully all what you explain me now will be useful for all the future contributions i may have (and i plan to have some!)

@satra
Copy link
Member

satra commented Jun 12, 2013

yup. the deprecation will kick in if you try to run an interface with an input that was deprecated using a nipype release greater than equal to the version indicated in the metadata. you can test it by saying deprecated= '0.8' - also deprecated='0.10'

finally, i think for this interface we need to remove exists=True for any of the outputs that are predicated on input settings.

(ps. no worries regarding time - i would rather teach somebody how to do something - it spreads the knowledge quicker).

@mick-d
Copy link
Contributor Author

mick-d commented Jun 12, 2013

Thanks for that.

I did several tests and, contrarily to what mentioned in my previous post, it seems that i need to use the new variable in the new line of code and not the old variable. So in the third post above the new line of code should actually be:

if isdefined(self.inputs.save_bias_corrected) and self.inputs.save_bias_corrected:
outputs['bias_corrected_image'] = fname_presuffix(f, prefix='m')

…ent (to be replaced with bias_corrected_image) and removed exists=True for outputs that are predicated on input settings
normalized_csf_image = File(desc='normalized csf probability map')
modulated_csf_image = File(desc='modulated, normalized csf probability map')
modulated_input_image = File(deprecated='0.10', new_name='bias_corrected_image', desc='bias-corrected version of input image')
bias_corrected_image = File(desc='bias-corrected version of input image')
transformation_mat = File(exists=True, desc='Normalization transformation')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only transformation_mat and inverse_transformation_mat are not predicated by inputs, so File(exists=True) remains for these outputs

@mick-d
Copy link
Contributor Author

mick-d commented Jun 13, 2013

Do you feel this PR is fine? Also i'd be happy to help working on the augmented fslmaths Interface you proposed

@satra
Copy link
Member

satra commented Jun 15, 2013

testing this now - should be good to go.

satra added a commit that referenced this pull request Jun 15, 2013
Correct Class Segment in spm.preprocess
@satra satra merged commit a251366 into nipy:master Jun 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants